refactor(cells): async cell allocate/free - #602
Conversation
Replace the `proxy_if_needed!` macro and make `CellsCache` async. `broadcast_kill` stays the only sync function, as it is called by `Drop`. Likewise, `do_free!` is replaced by `teardown_process_and_cgroup`. This now frees cells concurrently, so a host with many cells tears down in roughly the time of the slowest cell rather than the sum of all of them. No behavior change is expected.
| /// | ||
| /// Stays synchronous so [`Drop`] can call it. | ||
| pub fn kill(&mut self) -> Result<()> { | ||
| do_free!(self, kill(), broadcast_kill()) |
There was a problem hiding this comment.
do_free! was a useful common utility shared between free and kill. is there a benefit to removing it?
There was a problem hiding this comment.
Maybe a bit premature. This was a small change I was attempting to peel off as a prerequisite for a few more features I've been working on, like cell networking (issue / write-up coming!).
Once we're managing network devices, Cell::free will need to be async to make netlink calls. do_free! put the differing functions (sync/async) inside the expansion so I refactored the same logic into a function accepting a closure instead to satisfy this.
| // From here, we know the cgroup doesn't exist, so remove from cache | ||
| // if it does | ||
| if let Some(_removed) = self.cache.remove(&cell_name) { | ||
| // TODO: Should we not remove the cell (that has no cgroup) from |
There was a problem hiding this comment.
while you're here, can you think about whether this TODO makes sense? maybe we can implement it if you think it does or remove the comment if you think it doesn't.
There was a problem hiding this comment.
I was going through the scenarios in my head and sheepishly left this unresolved. Thinking out loud:
- We have exclusive control over auraed-managed cgroups due to the mutex over
Cellsso no threat of a race. - Since we prefix the names, I don't think cgroup name collision is possible for child auraeds.
- Something may have failed when it allocated it in the first place (or something else cleaned up the cgroup), so should we just re-allocate it?
- Similarly on line 100 - If we remove from cache on allocate failure, then the above case should only happen if something outside auraed removed the cgroup.
Maybe removing on failure to allocate + overwriting existing cell without a cgroup is sensible?
2 participants
Replace the
proxy_if_needed!macro and makeCellsCacheasync.broadcast_killstays the only sync function, as it is called byDrop. Likewise,do_free!is replaced byteardown_process_and_cgroup.This allows cells to free concurrently, so a host with many cells tears down in roughly the time of the slowest cell rather than the sum of all of them. This becomes important once we're tearing down network devices, etc.